Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update typescript setup #296

Closed
wants to merge 9 commits into from
Closed

Update typescript setup #296

wants to merge 9 commits into from

Conversation

benmccann
Copy link
Collaborator

@benmccann benmccann commented Dec 21, 2021 •
edited
Loading

  • Quick Checklist
  • I have read the contributing guidelines
  • I have written new tests, as applicable (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • I have added a changeset, if applicable
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Cleanup

  • What is the new behavior (if this is a feature change)?

None

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this
    PR?)

No

  • Other information:

rollup-plugin-typescript2 has tons of required peerDependencies that aren't installed. Switch to @rollup/plugin-typescript instead since it has fewer required peerDependencies and then install them (i.e. tslib). rollup-plugin-typescript2 was originally better than @rollup/plugin-typescript, but the latter has improved quite a bit and now is the better library and far better tested (rollup-plugin-typescript2 has no tests whatsoever)

This is necessary if we'd want to try out pnpm (#292) because pnpm validates that you have the required peedDependencies installed

The yarn.lock file is smaller with this change because @rollup/plugin-typescript pulls in fewer dependencies than rollup-plugin-typescript2 does, which is nice as well

@changeset-bot
Copy link

changeset-bot bot commented Dec 21, 2021 •
edited
Loading

⚠️ No Changeset found

Latest commit: 34fca85

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov
Copy link

codecov bot commented Dec 21, 2021 •
edited
Loading

Codecov Report

Merging #296 (b57b6c0) into main (aa13450) will not change coverage.
The diff coverage is n/a.

❗ Current head b57b6c0 differs from pull request most recent head 34fca85. Consider uploading reports for the commit 34fca85 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #296   +/-   ##
=======================================
  Coverage   94.60%   94.60%           
=======================================
  Files          30       30           
  Lines         426      426           
  Branches      103      103           
=======================================
  Hits          403      403           
  Misses         23       23
Flag Coverage Δ
imagetools-core 97.60% <ø> (ø)
rollup-plugin-imagetools 98.27% <ø> (ø)
vite-imagetools 80.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa13450...34fca85. Read the comment docs.

@JonasKruckenberg
Copy link
Owner

Does @eollup/plugin-typescript generate typings now? That was the only reason for rollup-plugin-typescript2.

@benmccann
Copy link
Collaborator Author

Yes, I believe it should: rollup/plugins#61

I'm not sure if there might be some additional configuration needed for that to happen or if it's already happening with this PR

@benmccann
Copy link
Collaborator Author

I've pushed a change to make it so that types are generated with this PR

JonasKruckenberg
JonasKruckenberg requested changes Dec 22, 2021
View reviewed changes
@JonasKruckenberg JonasKruckenberg changed the title Install necessary peer dependencies Update typescript setup Dec 22, 2021
@benmccann
Copy link
Collaborator Author

this is strange. I can't reproduce any failure locally eventhough the CI keeps failing

@JonasKruckenberg
Copy link
Owner

We love jest 🙃

@benmccann
Copy link
Collaborator Author

Maybe I'll finally have a reason to try out the new https://vitest.dev/

@JonasKruckenberg
Copy link
Owner

I ended up implementing this myself, to fix that annoying jest issue, but thanks for your PR anyway!
repo uses pnpm now 😉

@benmccann
Copy link
Collaborator Author

that's great! thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonasKruckenberg JonasKruckenberg JonasKruckenberg requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benmccann @JonasKruckenberg